Skip to content

Simplify ASan JS transform - #24314

Merged
sbc100 merged 1 commit into
emscripten-core:mainfrom
RReverser:simplify-asan-js
May 13, 2025
Merged

Simplify ASan JS transform#24314
sbc100 merged 1 commit into
emscripten-core:mainfrom
RReverser:simplify-asan-js

Conversation

@RReverser

@RReverser RReverser commented May 12, 2025

Copy link
Copy Markdown
Collaborator

Leverages ASan's C API for arbitrary loads/stores instead of custom C code with separate variants for each possible type.

Similar to #24295 and #24291.

@RReverser
RReverser requested review from kripken and sbc100 May 12, 2025 22:44
@RReverser RReverser mentioned this pull request May 12, 2025
Merged
kripken
kripken reviewed May 12, 2025
View reviewed changes
Comment thread src/runtime_asan.js
Comment thread src/runtime_asan.js
kripken
kripken approved these changes May 12, 2025
View reviewed changes

@kripken kripken left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

sbc100
sbc100 reviewed May 12, 2025
View reviewed changes
Comment thread tools/link.py
@RReverser
Simplify ASan JS transform
fd7826d 
Leverages ASan's C API for arbitrary loads/stores
instead of custom C code with variants for each
possible type.
@RReverser
RReverser enabled auto-merge (squash) May 13, 2025 00:13
@sbc100
sbc100 disabled auto-merge May 13, 2025 01:17
@sbc100
sbc100 merged commit 4607fb4 into emscripten-core:main May 13, 2025
@RReverser
RReverser deleted the simplify-asan-js branch May 13, 2025 01:19
@brendandahl

brendandahl commented May 13, 2025

Copy link
Copy Markdown
Collaborator

This appears to be causing a new failure on the rollers FAIL: test_stack (test_core.asan)

@RReverser

RReverser commented May 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

Sigh. I wish regular CI would cover all those checks (or that it would be at least possible to trigger them all manually before merging a PR). I'll look into it.

@sbc100

sbc100 commented May 13, 2025

Copy link
Copy Markdown
Collaborator

Looks like some kind of bad interaction between asan and STACK_OVERFLOW_CHECK=2:

RuntimeError: Aborted(stack overflow (Attempt to set SP to 0x12957410, with stack limits [0x12947420 - 0x12957420]). If you require more stack space build with -sSTACK_SIZE=<bytes>)
    at abort (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_stack.js:758:41)
    at ___handle_stack_overflow (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_stack.js:1138:3)
    at test_stack.wasm.__asan_loadN (wasm://wasm/test_stack.wasm-00146046:wasm-function[444]:0x1b976)
    at /usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_stack.js:773:12
    at _asan_js_load (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_stack.js:398:5)
    at checkStackCookie (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_stack.js:271:17)
    at initRuntime (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_stack.js:619:3)
    at doRun (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/test_stack.js:4579:5)

It looks like __asan_loadN is being used for the loads in checkStackCookie but probably shouldn't?

sbc100 added a commit to sbc100/emscripten that referenced this pull request May 13, 2025
@sbc100
Initialize the stack bounds prior to checking the cookie
383a2de 
Without this change reading of the cookie value calls into __asan_loadN
which then tries to use SP, but SP is not yet initialzed, so the
asan code itself trigger a STACK_OVERFLOW_CHECK error.

This fixes the asan.test_stack test which was broken by emscripten-core#24314 but went
unnoticed because we don't run all the asan tests in emscripten CI.
@sbc100 sbc100 mentioned this pull request May 13, 2025
Merged
@sbc100

sbc100 commented May 13, 2025

Copy link
Copy Markdown
Collaborator

I found a fix: #24317

@RReverser

RReverser commented May 13, 2025

Copy link
Copy Markdown
Collaborator Author

Oh nice, I only managed to narrow it down to GET_CALLER_PC_BP_SP in the ASAN implementation for now and "fixup" that locally. Yours sounds more promising though, does it fix all 6 failing tests?

@sbc100

sbc100 commented May 13, 2025

Copy link
Copy Markdown
Collaborator

I only verified that one test. Which other ones are failing?

@sbc100

sbc100 commented May 13, 2025

Copy link
Copy Markdown
Collaborator

Yup looks like it fixes all of them. The common theme was they all used -sSTACK_OVERFLOW_CHECK=2 (or -sASSERTIONS=2 which implies it)

sbc100 added a commit that referenced this pull request May 13, 2025
@sbc100
Initialize the stack bounds prior to checking the cookie (#24317)
870b3b2 
Without this change reading of the cookie value calls into __asan_loadN
which then tries to use SP, but SP is not yet initialzed, so the asan
code itself trigger a STACK_OVERFLOW_CHECK error.

This fixes the asan.test_stack test which was broken by #24314 but went
unnoticed because we don't run all the asan tests in emscripten CI.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbc100 sbc100 sbc100 left review comments
@kripken kripken kripken approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@RReverser @brendandahl @sbc100 @kripken